-
Notifications
You must be signed in to change notification settings - Fork 58
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add Spring Boot Project Starter for Google Gemini API model - ChatLangauge, Streaming model and Embedding Model #74
base: main
Are you sure you want to change the base?
Conversation
@langchain4j hey can you please have a look at this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Suhas-Koheda thanks a lot!
.topP(chatModelProperties.getTopP()) | ||
.topK(chatModelProperties.getTopK()) | ||
.maxOutputTokens(chatModelProperties.getMaxOutputTokens()) | ||
.responseFormat(ResponseFormat.JSON) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi. Related to your question. AutoConfig it's not injecting logging properties and json format from the properties -> https://docs.langchain4j.dev/tutorials/logging/ is not working when this AutoConfig is used. I will wait until this PR is merged to create a PR to avoid conflicts or If you want to add this configuration in this PR. Whatever you consider better.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! can you provide me with some references
As i did not find the logging used in other starters
Or maybe i overlooked
Thank you!
@franglopez
langchain4j-google-ai-gemini/src/main/java/dev/langchain4j/googleaigemini/AutoConfig.java
Outdated
Show resolved
Hide resolved
...ain4j-google-ai-gemini/src/main/java/dev/langchain4j/googleaigemini/ChatModelProperties.java
Outdated
Show resolved
Hide resolved
.../src/test/java/dev/langchain4j/googleaigemini/Langchain4jGoogleAiGeminiApplicationTests.java
Outdated
Show resolved
Hide resolved
langchain4j-google-ai-gemini/src/main/resources/application.properties.local
Outdated
Show resolved
Hide resolved
@langchain4j also in the timeout values i had doubt At present i have hardcoded used 60 secs |
@Suhas-Koheda you can see how timeouts are handled in OpenAI starter here, the same for .logRequests(chatModelProperties.logRequests()). |
@Suhas-Koheda |
Yeah sure I'll take care of it! |
@ddobrin hey for the response format i have kept the type of prop as ResponseFormat only which is user defined - not a primitive type? |
…urces/application.properties
@langchain4j |
@Suhas-Koheda on a 📱 let me come back to it at the beginning of the week |
If you like to add it and send a PR, that would be great |
@ddobrin hey i have opened a pr for the above in the main repo and also i have chnaged the tests here also to support the same ! |
Hi @Suhas-Koheda Right now, your tests are failing due to Bean instantiation issues.
and your tests would succeed. |
Yeah okay! |
completed! 🎉 |
All good @Suhas-Koheda cc/ @langchain4j |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Suhas-Koheda thank you!
@ddobrin could you please take a look at my comments as well?
...mini-spring-boot-starter/src/main/java/dev/langchain4j/googleaigemini/spring/AutoConfig.java
Outdated
Show resolved
Hide resolved
...mini-spring-boot-starter/src/main/java/dev/langchain4j/googleaigemini/spring/AutoConfig.java
Outdated
Show resolved
Hide resolved
...mini-spring-boot-starter/src/main/java/dev/langchain4j/googleaigemini/spring/AutoConfig.java
Outdated
Show resolved
Hide resolved
...mini-spring-boot-starter/src/main/java/dev/langchain4j/googleaigemini/spring/AutoConfig.java
Outdated
Show resolved
Hide resolved
...ng-boot-starter/src/main/java/dev/langchain4j/googleaigemini/spring/ChatModelProperties.java
Outdated
Show resolved
Hide resolved
...ot-starter/src/main/java/dev/langchain4j/googleaigemini/spring/EmbeddingModelProperties.java
Outdated
Show resolved
Hide resolved
...starter/src/main/java/dev/langchain4j/googleaigemini/spring/GeminiFunctionCallingConfig.java
Outdated
Show resolved
Hide resolved
...ng-boot-starter/src/main/java/dev/langchain4j/googleaigemini/spring/GeminiSafetySetting.java
Outdated
Show resolved
Hide resolved
@langchain4j done! |
@Suhas-Koheda
with
Thanks |
@ddobrin hey i have completed it! |
Hi @Suhas-Koheda The change for "supporting multiple models in the same config" is separate but it's good that it surfaced in the context of AutoConfig. |
Yeah! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Suhas-Koheda thanks a lot!🙏
I've applied a few cosmetic changes, I hope you don't mind.
I've also noticed that when safetySettings or toolConfig is not specified explicitly in the properties, the whole thing fails with NPE, could you please fix this? We can merge after this is fixed. This PR will be a great addition to the next relesase which is planned for this week (on Thursday)!
.responseFormat(chatModelProperties.responseFormat()) | ||
.logRequestsAndResponses(chatModelProperties.logRequestsAndResponses()) | ||
.safetySettings(Map.of( | ||
// TODO NPE? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rewrite it to avoid NPE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@langchain4j hey how can we avoid NPE like i am writing a function that checks if it is empty or not
if it is not empty the map is returned or else i wanted to return a defautl value
but what can the default value be
@ddobrin if you can help me with this :-)
Thank you
chatModelProperties.safetySetting().geminiHarmBlockThreshold() | ||
)) | ||
.toolConfig( | ||
// TODO NPE? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, please check all other model beans as well
@Suhas-Koheda I would love to include your PR in this release (today), but there are still some issues left (please see my comments above). Since this is an independent module, we can release it a bit later separately, once this PR is ready. |
@langchain4j |
@langchain4j hey i have written the NPE checks can you please have a look over this |
return defaultMap; | ||
} | ||
return Map.of( | ||
safetySetting.geminiHarmCategory(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these settings supposed to have only a single key-value pair?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The safety settings builder takes the map of geminiharmcategory as key and geminiharmblockthreshold as value
No it can take any number of values
It changes them into list
But for a single model thre would be only one harmcategory and blockthreshold right?
If that's not the case we can write such that the harmcategory and blockthreshold takes comma separated values and we manually convert then into map in the autoconfig?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private Map<GeminiHarmCategory,GeminiHarmBlockThreshold> checkSafetySettingForNull(GeminiSafetySetting safetySetting) {
if(safetySetting==null){
Map<GeminiHarmCategory,GeminiHarmBlockThreshold> defaultMap= new HashMap<>();
defaultMap.put(HARM_CATEGORY_CIVIC_INTEGRITY,HARM_BLOCK_THRESHOLD_UNSPECIFIED);
return defaultMap;
}
Map<GeminiHarmCategory,GeminiHarmBlockThreshold> userMap= new HashMap<>();
safetySetting.geminiHarmCategory().forEach(category -> userMap.put(category,safetySetting.geminiHarmBlockThreshold().get(safetySetting.geminiHarmCategory().indexOf(category))));
return userMap;
}
this can be done if there are multiple GeminiHarmCategory and GeminiHarmBlockThreshold
and the test cases run properly too
the code is ready to be commited
thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ddobrin, could you please help with the review?
It seems to me that current logic in checkSafetySettingForNull
is not correct. If settings are not specified, defaultMap.put(HARM_CATEGORY_CIVIC_INTEGRITY,HARM_BLOCK_THRESHOLD_UNSPECIFIED);
is set (is it correct?). Also, chatModelProperties
can have only a single setting...
@Suhas-Koheda I guess ChatModelProperties
should have a Map<GeminiHarmCategory, GeminiHarmBlockThreshold> safetySettings
instead of GeminiSafetySetting safetySetting
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or List<GeminiSafetySetting> safetySettings
, the same way as it is in the BaseGeminiChatModel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Suhas-Koheda I guess
ChatModelProperties
should have aMap<GeminiHarmCategory, GeminiHarmBlockThreshold> safetySettings
instead ofGeminiSafetySetting safetySetting
.
Yeah this can be done so that while setting in the properties file it will be in a more understanable way
yeah ill change it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The safety filters settings, with their defaults, are available at his link
For flash and pro 1.5 the "default" block method is "SEVERITY" with the threshold by default at "BLOCK_MEDIUM_AND_ABOVE".
The HARM_CATEGORY_CIVIC_INTEGRITY filter is off by default.
Can I suggest that you do not set a value at all in
private GeminiMode checkGeminiModeForNull(GeminiFunctionCallingConfig geminiFunctionCallingConfig)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To simplify, you can use the GeminiSafetySetting class which has a pair of <category, threshold?
@langchain4j hey! :-) |
@langchain4j this logic looks perfect to me now! |
Let me have a look tomorrow |
Closes langchain4j/langchain4j#2103